Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CompatHelper: bump compat for CxxWrap to 0.16 #66

Merged

Conversation

github-actions[bot]
Copy link
Contributor

This pull request changes the compat entry for the CxxWrap package from 0.14 to 0.14, 0.16.
This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.

@github-actions github-actions bot force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from c8c4914 to 0c9af17 Compare June 10, 2024 20:55
@giordano giordano changed the title CompatHelper: bump compat for CxxWrap to 0.16, (keep existing compat) CompatHelper: bump compat for CxxWrap to 0.16 Jun 12, 2024
@giordano giordano marked this pull request as draft June 12, 2024 20:13
@giordano
Copy link
Collaborator

@barche would you happen to be able to help with the upgrade? Building the wrappers now fails when switching to latest cxxwrap.

@barche
Copy link

barche commented Jun 12, 2024

I think this is a libcxxwrap bug actually.

@PraneethJain from the logs this seems to be caused by std::set being applied to an incompatible type: https://github.com/JuliaIPU/IPUToolkit.jl/actions/runs/9489008777/job/26149313737?pr=66#step:8:237

/github/home/.julia/artifacts/0c7b615ac941d356502c54c3f343cc97f247f3c1/include/jlcxx/stl.hpp:272:83:   required from ‘void jlcxx::stl::WrapSetType::operator()(TypeWrapperT&&) [with TypeWrapperT = jlcxx::TypeWrapper<std::set<std::pair<int, poplar::program::Program>, std::less<std::pair<int, poplar::program::Program> >, std::allocator<std::pair<int, poplar::program::Program> > > >]’

Any ideas how this can happen? I am not able to reproduce this in a libcxxwrap test.

@PraneethJain
Copy link

PraneethJain commented Jun 13, 2024

Lexicographic comparison for std::pair using operator< was removed in C++20 (https://en.cppreference.com/w/cpp/utility/pair/operator_cmp), which is why this is not reproducible in libcxxwrap.

The reason for the test failure here could possibly be because the library is compiled using C++17 (https://github.com/JuliaIPU/IPUToolkit.jl/actions/runs/9489008777/job/26149313737?pr=66#step:8:1903), where std::pair is still lexicographically comparable using operator< and thus std::set is being applied to it, even though the type is incompatible.

@giordano
Copy link
Collaborator

@PraneethJain are you suggesting that compiling with -std=c++20 would help? Or do we still need to change something somewhere?

@PraneethJain
Copy link

@PraneethJain are you suggesting that compiling with -std=c++20 would help? Or do we still need to change something somewhere?

Hi, yes compiling on C++20 might solve the issue. In case that doesn't work, I can explicitly handle the case of std::pair with operator< in libcxxwrap so as to make it work on both versions.

@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from 94fdfe0 to e2a4ea5 Compare June 13, 2024 16:59
@giordano
Copy link
Collaborator

Using -std=c++2a doesn't seem to be enough, I'm still getting https://github.com/JuliaIPU/IPUToolkit.jl/actions/runs/9503721078/job/26194782493#step:8:162

/usr/include/c++/9/bits/stl_pair.h:456:50: error: no match for 'operator<' (operand types are 'const poplar::program::Program' and 'const poplar::program::Program')

@PraneethJain
Copy link

PraneethJain commented Jun 14, 2024

Using -std=c++2a doesn't seem to be enough, I'm still getting JuliaIPU/IPUToolkit.jl/actions/runs/9503721078/job/26194782493#step:8:162

/usr/include/c++/9/bits/stl_pair.h:456:50: error: no match for 'operator<' (operand types are 'const poplar::program::Program' and 'const poplar::program::Program')

@giordano C++20 features aren't fully implemented on g++ 9, the <=> operator is only supported on g++ 10 and onwards (https://en.cppreference.com/w/cpp/compiler_support/20), and the logs show that the package is being built using g++ 9.

image

Upgrading to g++ 10 and then using C++20 should solve the issue.

JuliaInterop/libcxxwrap-julia#163 makes it so that this would work across versions.

@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from e2a4ea5 to dd55667 Compare June 14, 2024 17:55
@giordano
Copy link
Collaborator

Ok, by switching to G++ 10 does get me past the building error, but now CxxWrap itself can't be precompiled:

Failed to precompile CxxWrap [1f15a43c-97ca-5a2a-ae31-89f07a497df4] to "/github/home/.julia/compiled/v1.9/CxxWrap/jl_WOVH75".
ERROR: LoadError: UndefRefError: access to undefined reference

Also:

  /github/home/.julia/artifacts/3db338517ddd86990fb766f5488d535490e1d52f/include/jlcxx/module.hpp:124 could not find default C++ compiler in PATH: c++

It looks like CxxWrap assumes the compiler is called c++, but in this container is now called g++-10.

@giordano
Copy link
Collaborator

I rerun the tests with latest version of libcxxwrap_julia, but I'm still getting

  /github/home/.julia/artifacts/a171557f021273c0bad6f9a8d54db6f9da7e99b0/include/jlcxx/module.hpp:124 could not find default C++ compiler in PATH: c++

I tried to grep the error message in libcxxwrap but I couldn't find it, I have no idea where that's coming from. Is there a way to change the expected compiler name?

@barche
Copy link

barche commented Jun 18, 2024

With libcxxwrap 0.13.2 you should be able to revert to GCC 9 and the c++17 standard. You may also need to add -DJLCXX_FORCE_RANGES_OFF to the compile options if errors about std::ranges not being defined appear.

I don't know where the error about finding the compiler comes from, normally finding the compiler is handled by cmake but that is not used here. It would probably be more robust to switch to a CMake-based approach, but I don't think it's really needed if the above suggestion works.

@giordano
Copy link
Collaborator

But I'm very confused by why this is happening at runtime at all?

@giordano
Copy link
Collaborator

Just to be super clear: the error isn't happening during compilation of the wrapper, that step is now successful, instead the error message pops up during evaluation of Julia code when running the tests of the package, which is what makes it annoying. I don't see what is looking for a C++ compiler to start with.

@barche
Copy link

barche commented Jun 18, 2024

As far as I can tell, this is poplar itself trying to compile some code that is generated in that test. Simply installing GCC9 in addition to GCC10 may fix this, or somehow changing the default compiler setting, but a quick search wasn't helpful in figuring out how.

@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from dd55667 to 5cd080a Compare June 18, 2024 18:41
@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from 5cd080a to 7688bac Compare June 18, 2024 19:00
@giordano
Copy link
Collaborator

Ok, thanks, I confirm this probably comes from poplar, grepping the entire SDK finds a match in libpoplar.so, I was quite confused by the error message mentioning cxxwrap 🥲 Thanks for the help!

@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from 7688bac to ceb0cad Compare June 18, 2024 19:30
@giordano giordano marked this pull request as ready for review June 18, 2024 19:37
@giordano giordano force-pushed the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch from ceb0cad to f9e1a12 Compare June 18, 2024 19:38
@giordano giordano merged commit ccc76bd into main Jun 18, 2024
2 checks passed
@giordano giordano deleted the compathelper/new_version/2024-06-10-20-55-30-376-04162310075 branch June 18, 2024 20:07
@giordano
Copy link
Collaborator

@barche Do I understand correctly that -DJLCXX_FORCE_RANGES_OFF is required with GCC 10+?

I think I misunderstood the use of -DJLCXX_FORCE_RANGES_OFF before: I thought it may have been needed only for GCC 9 and it was working without, but I just tried compiling the wrapper with GCC 11 and ran into the undefined std::ranges error, which was fixed by using -DJLCXX_FORCE_RANGES_OFF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants